feat: emit and parse durations as ISO-8601 in launch form converter#7596
Open
ursucarina wants to merge 1 commit into
Open
feat: emit and parse durations as ISO-8601 in launch form converter#7596ursucarina wants to merge 1 commit into
ursucarina wants to merge 1 commit into
Conversation
Signed-off-by: Carina Ursu <carina@union.ai>
EngHabu
reviewed
Jun 28, 2026
EngHabu
left a comment
EngHabu
left a comment
Contributor
There was a problem hiding this comment.
The error I get even when I write 2h. Which is a valid time.ParseDuration value I thought. So I'm a bit confused
| // formatISO8601Duration converts a time.Duration into an ISO-8601 duration string | ||
| // (e.g. "P2DT3H4M5S"). Only days/hours/minutes/seconds are emitted (never years/months) so the | ||
| // representation is unambiguous; the launch form frontend renders it back to a readable string. | ||
| func formatISO8601Duration(d time.Duration) string { |
Contributor
There was a problem hiding this comment.
We should use a library for this
|
|
||
| // parseISO8601Duration parses an ISO-8601 duration string (e.g. "P2DT3H4M5S") into a | ||
| // durationpb.Duration. Returns an error if the string is not a valid ISO-8601 duration. | ||
| func parseISO8601Duration(s string) (*durationpb.Duration, error) { |
Contributor
There was a problem hiding this comment.
Can't we usea library instead?
channelmeter/iso8601duration
Maybe...
Contributor
Author
There was a problem hiding this comment.
From me looking:
Short answer: we could, but channelmeter/iso8601duration is a poor fit and would actually be a regression. It's not currently a dependency either, so we'd be adding a new (unmaintained) module.
The two dealbreakers:
- It doesn't cover the formatting direction. String() only formats the library's own Duration struct (Years/Weeks/Days/Hours/Minutes/Seconds), not a Go time.Duration. Our literalToJsonValues path needs time.Duration → ISO string, so we'd still hand-roll the decomposition — the library buys us nothing there. By its own docstring it's a "partial implementation of ISO8601 durations (no months)."
- It drops cases we explicitly support. It rejects months, can't represent fractional seconds, and has no negatives. Our converter handles all three. Swapping it in would silently narrow behavior and break frontend round-tripping (dayjs can emit P1M).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why are the changes needed?
Launch form duration fields are declared in JSON Schema with
format: "duration", which per the spec means an ISO-8601 duration string (e.g.P2DT3H). The converter instead emitted Go-style strings (time.Duration.String(), e.g.2h30m) and only parsed bare-seconds or Go-style strings on the way back in. This made the converter'sDURATIONhandling inconsistent with theformat: "duration"schema it produces, and it rejected ISO-8601 input.What changes were proposed in this pull request?
This makes the launch form converter (
runs/service/converter/literal_json_converter.go) speak ISO-8601 forDURATION:literalToJsonValuesnow formats durations as ISO-8601 (formatISO8601Duration) instead ofDuration.String().jsonValueToLiteral(theSimpleType_DURATIONcase) now parses ISO-8601 first, then falls back to bare seconds and Go-style duration strings for backward compatibility.isISO8601Duration/parseISO8601Duration/formatISO8601Durationhelpers. Year and month lengths are fixed (365-day years, 30-day months) so durations round-trip deterministically.How was this patch tested?
Unit tests in
runs/service/converter/literal_json_converter_test.go:PT1H30M).TestFormatISO8601Duration,TestParseISO8601Duration(valid, invalid, and format↔parse round-trip cases), andTestJSONValuesToLiteralsDurationcovering ISO-8601 plus the backward-compatible bare-seconds and Go-style inputs.go test ./runs/service/converter/passes.Labels
Check all the applicable boxes